Conversation
📝 WalkthroughWalkthroughThe PR adds terms agreement tracking to the signup flow by introducing an Changes
Sequence DiagramsequenceDiagram
participant Client
participant AuthUserCommandService
participant AuthenticationEvent
participant MemberEventListener
participant MemberCommandService
participant TermsRepository
participant MemberTermsRepository
Client->>AuthUserCommandService: signUp(email, password, agreedTermsIds)
AuthUserCommandService->>AuthUserCommandService: email verification & user save
AuthUserCommandService->>AuthenticationEvent: CreateMember(id, email, agreedTermsIds)
AuthUserCommandService->>MemberEventListener: publish event
MemberEventListener->>MemberCommandService: createMember(memberId, email, agreedTermsIds)
MemberCommandService->>MemberCommandService: validateRequiredTerms(agreedTermsIds)
MemberCommandService->>TermsRepository: fetch all required terms
alt Required terms validation fails
MemberCommandService->>MemberCommandService: throw REQUIRED_TERMS_NOT_AGREE
else Validation succeeds
MemberCommandService->>TermsRepository: fetch each agreed Term by id
MemberCommandService->>MemberTermsRepository: save MemberTerms for each agreed term
end
MemberCommandService-->>MemberEventListener: return member
MemberEventListener-->>Client: signup complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @chaechaen, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 사용자 회원가입 과정에 필수 약관 동의 기능을 통합하여, 사용자가 서비스 이용을 위한 필수 약관에 동의해야만 회원가입을 완료할 수 있도록 시스템을 강화합니다. 이를 통해 서비스의 법적 요구사항을 준수하고 사용자 동의 내역을 체계적으로 관리할 수 있게 됩니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/main/java/checkmo/authentication/AuthenticationEvent.java`:
- Around line 8-10: The CreateMember event built in CustomOAuth2UserService
omits agreedTermsIds, causing MemberCommandService.createMember's validation (if
(agreedTermsIds == null || !agreedTermsIds.containsAll(requiredTermsIds))) to
throw; fix by supplying a non-null list when constructing
CustomOAuth2UserService.CreateMember (either the actual agreed terms if social
login should require them or an explicit empty list like
Collections.emptyList()/List.of() to skip validation), updating the builder call
in CustomOAuth2UserService to set agreedTermsIds accordingly so
MemberCommandService.createMember receives a non-null value.
In
`@src/main/java/checkmo/member/internal/service/command/MemberCommandService.java`:
- Around line 38-39: In createMember, deduplicate the incoming agreedTermsIds
immediately (e.g., produce a distinctTermsIds collection) and use that for
validation and all subsequent logic so you don't create duplicate MemberTerms
rows; call validateRequiredTerms(distinctTermsIds) (instead of
validateRequiredTerms(agreedTermsIds)) and replace further usages of
agreedTermsIds in MemberTerms creation and related code with distinctTermsIds to
ensure each term results in at most one MemberTerms entry.
In `@src/main/resources/db/migration/V20260211__insert_default_terms.sql`:
- Around line 1-4: Add a trailing newline to the migration file so it ends with
a newline character and ensure the INSERT INTO terms statement is terminated
properly; additionally, if your project prefers not to hardcode PKs, remove the
explicit id values from the INSERT INTO terms (...) VALUES (...) (or adjust to
use DEFAULT for id) so GenerationType.IDENTITY/AUTO_INCREMENT manages IDs, or
add a sequence-reset statement after inserting fixed rows to keep auto-increment
in sync — update the INSERT INTO terms and any post-insert sequence maintenance
accordingly.
🧹 Nitpick comments (3)
src/main/java/checkmo/authentication/web/dto/AuthRequestDTO.java (1)
29-31: Consider adding@Schemaannotation for API documentation consistency.Other fields in this DTO have
@Schemaannotations for Swagger docs. Adding one here (e.g.,@Schema(description = "동의한 약관 ID 목록", example = "[1, 2]")) would keep the API docs consistent.src/main/java/checkmo/member/internal/service/command/MemberCommandService.java (2)
54-65: N+1 queries: batch-fetch terms and batch-save MemberTerms.Each iteration issues a separate
findByIdandsave. Fornagreed terms this means2nqueries. Fetch all agreed terms in one call and usesaveAll.♻️ Proposed refactor
- // 여기서 약관 내역 DB에 저장 (멤버 생성 후) - for (Long termsId : agreedTermsIds) { - Terms terms = termsRepository.findById(termsId) - .orElseThrow(() -> new MemberException(MemberErrorStatus.TERMS_NOT_FOUND)); - - MemberTerms memberTerms = MemberTerms.builder() - .member(member) - .terms(terms) - .isAgreed(true) - .build(); - - memberTermsRepository.save(memberTerms); - } + List<Terms> termsList = termsRepository.findAllById(agreedTermsIds); + if (termsList.size() != agreedTermsIds.size()) { + throw new MemberException(MemberErrorStatus.TERMS_NOT_FOUND); + } + + List<MemberTerms> memberTermsList = termsList.stream() + .map(terms -> MemberTerms.builder() + .member(member) + .terms(terms) + .isAgreed(true) + .build()) + .toList(); + + memberTermsRepository.saveAll(memberTermsList);
68-77: Two concerns:findAll()is overly broad, and duplicate IDs are not guarded.
findAll()loads every terms row just to filter required ones. Add a repository method likefindByIsRequiredTrue()(or a@Queryselecting whererequired = true) to push the filter to the DB.- If
agreedTermsIdscontains duplicates,containsAllstill passes but duplicateMemberTermsrows will be persisted. Deduplicate early withnew ArrayList<>(new HashSet<>(agreedTermsIds))or add a unique constraint on(member, terms).♻️ Proposed refactor for validation
private void validateRequiredTerms(List<Long> agreedTermsIds) { - List<Long> requiredTermsIds = termsRepository.findAll().stream() - .filter(Terms::isRequired) - .map(Terms::getId) - .toList(); + List<Long> requiredTermsIds = termsRepository.findByIsRequiredTrue().stream() + .map(Terms::getId) + .toList(); - if (agreedTermsIds == null || !agreedTermsIds.containsAll(requiredTermsIds)) { + if (agreedTermsIds == null || !new HashSet<>(agreedTermsIds).containsAll(requiredTermsIds)) { throw new MemberException(MemberErrorStatus.REQUIRED_TERMS_NOT_AGREE); } }Add to
TermsRepository:List<Terms> findByIsRequiredTrue();
| @Builder | ||
| public record CreateMember(String id, String email) { | ||
| public record CreateMember(String id, String email, List<Long> agreedTermsIds) { | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "CreateMember" --type java -C3Repository: checkmo2025/BE
Length of output: 3426
🏁 Script executed:
rg -n "memberCommandService.createMember|createMember\(" --type java -C5 src/main/java/checkmo/member/Repository: checkmo2025/BE
Length of output: 2314
🏁 Script executed:
rg -n "validateRequiredTerms" --type java -A10 src/main/java/checkmo/member/Repository: checkmo2025/BE
Length of output: 2758
Fix social login signup: supply agreedTermsIds in CustomOAuth2UserService.CreateMember builder.
Social login flow in CustomOAuth2UserService (line 56) publishes CreateMember without agreedTermsIds, causing it to be null. The event listener then calls MemberCommandService.createMember(), which validates required terms at line 74: if (agreedTermsIds == null || !agreedTermsIds.containsAll(requiredTermsIds)) throws MemberException. This breaks the social login signup flow. Either supply the agreed terms list (if social login should require terms agreement) or an empty list (if social login should skip this validation).
🤖 Prompt for AI Agents
In `@src/main/java/checkmo/authentication/AuthenticationEvent.java` around lines 8
- 10, The CreateMember event built in CustomOAuth2UserService omits
agreedTermsIds, causing MemberCommandService.createMember's validation (if
(agreedTermsIds == null || !agreedTermsIds.containsAll(requiredTermsIds))) to
throw; fix by supplying a non-null list when constructing
CustomOAuth2UserService.CreateMember (either the actual agreed terms if social
login should require them or an explicit empty list like
Collections.emptyList()/List.of() to skip validation), updating the builder call
in CustomOAuth2UserService to set agreedTermsIds accordingly so
MemberCommandService.createMember receives a non-null value.
| public void createMember(String memberId, String email, List<Long> agreedTermsIds) { | ||
| validateRequiredTerms(agreedTermsIds); |
There was a problem hiding this comment.
Consider deduplicating agreedTermsIds at entry to prevent duplicate MemberTerms rows.
If the caller passes [1, 1, 2], two MemberTerms rows are created for terms 1. Dedup early:
- public void createMember(String memberId, String email, List<Long> agreedTermsIds) {
- validateRequiredTerms(agreedTermsIds);
+ public void createMember(String memberId, String email, List<Long> agreedTermsIds) {
+ List<Long> distinctTermsIds = agreedTermsIds != null
+ ? agreedTermsIds.stream().distinct().toList()
+ : null;
+ validateRequiredTerms(distinctTermsIds);Then use distinctTermsIds for the rest of the method.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void createMember(String memberId, String email, List<Long> agreedTermsIds) { | |
| validateRequiredTerms(agreedTermsIds); | |
| public void createMember(String memberId, String email, List<Long> agreedTermsIds) { | |
| List<Long> distinctTermsIds = agreedTermsIds != null | |
| ? agreedTermsIds.stream().distinct().toList() | |
| : null; | |
| validateRequiredTerms(distinctTermsIds); |
🤖 Prompt for AI Agents
In
`@src/main/java/checkmo/member/internal/service/command/MemberCommandService.java`
around lines 38 - 39, In createMember, deduplicate the incoming agreedTermsIds
immediately (e.g., produce a distinctTermsIds collection) and use that for
validation and all subsequent logic so you don't create duplicate MemberTerms
rows; call validateRequiredTerms(distinctTermsIds) (instead of
validateRequiredTerms(agreedTermsIds)) and replace further usages of
agreedTermsIds in MemberTerms creation and related code with distinctTermsIds to
ensure each term results in at most one MemberTerms entry.
| INSERT INTO terms (id, term_url, is_required) VALUES | ||
| (1, 'https://www.notion.so/2cd7445c73f780dab5c5ccfbf5ce6101', true), -- 서비스 이용을 위한 필수 개인정보 수집 이용 동의(필수) | ||
| (2, 'https://www.notion.so/2cd7445c73f780e3a653ead96321331f', false), -- 마케팅 및 이벤트 정보 수신 동의 (선택) | ||
| (3, 'https://www.notion.so/3-2cd7445c73f7806f9b37f75eea6acb30', false); -- 개인정보 제3자 제공 동의(선택) No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and check Terms entity for ID generation strategy
find . -type f -name "Terms.java" | head -5 | xargs -I {} sh -c 'echo "=== File: {} ===" && cat -n {}'Repository: checkmo2025/BE
Length of output: 1048
🏁 Script executed:
#!/bin/bash
# Find CREATE TABLE migration for terms
rg -n "CREATE TABLE" --type sql -A10 | grep -A10 "terms"Repository: checkmo2025/BE
Length of output: 2687
🏁 Script executed:
#!/bin/bash
# Check the exact last character(s) of the V20260211 file to confirm newline
hexdump -C src/main/resources/db/migration/V20260211__insert_default_terms.sql | tail -3Repository: checkmo2025/BE
Length of output: 103
🏁 Script executed:
#!/bin/bash
# Check trailing newline using od
tail -c 20 src/main/resources/db/migration/V20260211__insert_default_terms.sql | od -cRepository: checkmo2025/BE
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Check if other seed data migrations also use explicit IDs
rg -n "INSERT INTO" --type sql -A1 src/main/resources/db/migration/ | head -40Repository: checkmo2025/BE
Length of output: 966
🏁 Script executed:
#!/bin/bash
# Check trailing newline using tail with -c and wc
file="src/main/resources/db/migration/V20260211__insert_default_terms.sql"
last_char=$(tail -c 1 "$file" | wc -l)
echo "Lines at end: $last_char"
# Also show last few characters using sed
echo "Last 5 chars:"
tail -c 5 "$file" | sed 's/.$/[END]/g' | cat -ARepository: checkmo2025/BE
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Check all INSERT migrations to see if hardcoded IDs are the standard pattern
find src/main/resources/db/migration -name "*.sql" -type f -exec grep -l "INSERT INTO" {} \; | while read f; do
echo "=== $f ==="
grep -A2 "INSERT INTO" "$f" | head -6
doneRepository: checkmo2025/BE
Length of output: 863
Add trailing newline to the file; reconsider hardcoded IDs if sequence management is a project concern.
The file is missing a trailing newline (line 4 ends without one).
Regarding the hardcoded IDs (1, 2, 3): The Terms entity uses GenerationType.IDENTITY with AUTO_INCREMENT on the id column. Explicitly inserting IDs can technically advance the sequence; however, for static reference data like these three terms, hardcoded IDs are a standard pattern to ensure consistency across environments. This is only problematic if the application later inserts Terms via JPA without specifying IDs and sequence management becomes inconsistent. If your project avoids explicit IDs in seed migrations, consider using IDENTITY generation without pre-set values or resetting the sequence after insertion.
🤖 Prompt for AI Agents
In `@src/main/resources/db/migration/V20260211__insert_default_terms.sql` around
lines 1 - 4, Add a trailing newline to the migration file so it ends with a
newline character and ensure the INSERT INTO terms statement is terminated
properly; additionally, if your project prefers not to hardcode PKs, remove the
explicit id values from the INSERT INTO terms (...) VALUES (...) (or adjust to
use DEFAULT for id) so GenerationType.IDENTITY/AUTO_INCREMENT manages IDs, or
add a sequence-reset statement after inserting fixed rows to keep auto-increment
in sync — update the INSERT INTO terms and any post-insert sequence maintenance
accordingly.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이번 PR은 회원가입 시 필수 약관 동의 기능을 추가하는 변경사항을 담고 있습니다. 하지만 MemberCommandService에서 높은 심각도의 서비스 거부(DoS) 취약점이 발견되었습니다. 현재 구현은 사용자가 제공하는 많은 수의 약관 동의 ID로 인해 과도한 데이터베이스 쿼리를 유발할 수 있으며, 이는 데이터베이스를 과부하시켜 애플리케이션 가용성에 영향을 줄 수 있습니다. 또한, 약관 데이터 처리 과정에서 발생할 수 있는 N+1 문제와 비효율적인 데이터 필터링 및 검증 로직에 대한 성능 개선점이 있습니다. 이 문제들을 해결하면 더 안정적이고 확장성 있는 코드가 될 것입니다.
| for (Long termsId : agreedTermsIds) { | ||
| Terms terms = termsRepository.findById(termsId) | ||
| .orElseThrow(() -> new MemberException(MemberErrorStatus.TERMS_NOT_FOUND)); | ||
|
|
||
| MemberTerms memberTerms = MemberTerms.builder() | ||
| .member(member) | ||
| .terms(terms) | ||
| .isAgreed(true) | ||
| .build(); | ||
|
|
||
| memberTermsRepository.save(memberTerms); | ||
| } |
There was a problem hiding this comment.
이 코드는 서비스 거부(DoS) 공격에 취약합니다. createMember 메서드는 사용자 제어 하에 있는 agreedTermsIds 목록을 반복하며 각 ID에 대해 데이터베이스 쿼리를 수행합니다. 공격자가 매우 많은 수의 ID를 제공하면 과도한 데이터베이스 쿼리가 발생하여 데이터베이스 리소스가 고갈되고 애플리케이션이 다른 사용자에게 제공되지 않을 수 있습니다. 현재 약관 동의 내역을 저장하는 로직은 동의한 약관의 수만큼 데이터베이스 조회가 발생하여 N+1 문제가 발생할 수 있습니다. findAllById와 saveAll을 사용하여 여러 약관을 한 번에 조회하고 저장하는 것이 성능에 더 효율적입니다.
List<Terms> allTerms = termsRepository.findAllById(agreedTermsIds);
if (allTerms.size() != agreedTermsIds.size()) {
throw new MemberException(MemberErrorStatus.TERMS_NOT_FOUND);
}
List<MemberTerms> memberTermsToSave = allTerms.stream()
.map(term -> MemberTerms.builder()
.member(member)
.terms(term)
.isAgreed(true)
.build())
.collect(Collectors.toList());
memberTermsRepository.saveAll(memberTermsToSave);| List<Long> requiredTermsIds = termsRepository.findAll().stream() | ||
| .filter(Terms::isRequired) | ||
| .map(Terms::getId) | ||
| .toList(); | ||
|
|
||
| if (agreedTermsIds == null || !agreedTermsIds.containsAll(requiredTermsIds)) { | ||
| throw new MemberException(MemberErrorStatus.REQUIRED_TERMS_NOT_AGREE); | ||
| } |
There was a problem hiding this comment.
필수 약관을 검증하는 로직이 비효율적입니다.
termsRepository.findAll()은 모든 약관을 DB에서 조회하므로 약관이 많아지면 성능 저하를 유발할 수 있습니다.TermsRepository에findAllByIsRequired(true)와 같은 메서드를 추가하여 DB에서 직접 필터링하는 것이 좋습니다.List.containsAll은 시간 복잡도가 O(n*m)으로 비효율적입니다.agreedTermsIds를HashSet으로 변환하여containsAll을 호출하면 평균 시간 복잡도를 O(n+m)으로 개선할 수 있습니다.
List<Long> requiredTermsIds = termsRepository.findAllByIsRequired(true).stream()
.map(Terms::getId)
.toList();
if (agreedTermsIds == null || !new HashSet<>(agreedTermsIds).containsAll(requiredTermsIds)) {
throw new MemberException(MemberErrorStatus.REQUIRED_TERMS_NOT_AGREE);
}1 participant
🚀 변경사항
회원가입 시 필수 약관 동의 저장 로직 추가 (1번 필수 약관 동의 안 하면 회원가입 못함)
🔗 관련 이슈
✅ 체크리스트
📝 특이사항
Summary by CodeRabbit
New Features
Chores